Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add path_prefix option to lint_package for GitHub Actions annotations #845

Merged
merged 10 commits into from
Oct 9, 2021

Conversation

jonkeane
Copy link
Contributor

@jonkeane jonkeane commented Aug 2, 2021

In order to get GitHub annotations to show up on a PR inline in the code, the annotation path must be relative to the repository root rather than relative to the package root. For most R packages, the package root is the same as the GitHub root, but for packages that use a subdirectory (e.g. https://github.com/apache/arrow in the r/ subdirectory) the annotations don't display without adding a prefix like this.

This PR doesn't change the default case, but adds an argument to lint_package() that allows someone to specify the correct prefix.

R/lint.R Outdated Show resolved Hide resolved
R/lint.R Outdated
@@ -259,6 +259,8 @@ lint_dir <- function(path = ".", relative_path = TRUE, ..., exclusions = list("r
#' Apply one or more linters to all of the R files in a package.
#' @param path the path to the base directory of the package, if \code{NULL},
#' it will be searched in the parent directories of the current directory.
#' @param path_prefix a prefix to add to the path of any file found, useful for
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the idea of what you're after & it makes sense, I'm less confident this is the best API for it. Maybe use project_dir instead, and then path is given relative to that?

cc @russHyde and @AshesITR for an API decision

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback, I would be happy to do that if that's the direction y'all would like.

For what it's worth, when we run these in our CI, we actually run them from the subdirectory, so having path_prefix / project_dir only add on to the output of the GHA annotations is the behavior we want (of course we could change where we lint from easily, so that's not a great argument for this API). If we did keep this behavior, we might also consider naming it something like annotation_path_prefix to make it's clear it's only about annotations and not where to run the file.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this is not really related to linting per se, but more to the print.lints method for GitHub.

@jonkeane
Copy link
Contributor Author

Sorry for the delay on this! Thanks for the comments. I've changed the UX here using an option (similar to what's already used in the print.lints method). I also updated the naming to reflect the project_dir suggestion.

README.md Outdated
@@ -208,6 +208,8 @@ location, namely in the `.github/workflows` directory of your repository. This f
pull request with the lints found and they will also be printed as [annotations](https://docs.github.com/en/free-pro-team@latest/github/collaborating-with-issues-and-pull-requests/about-status-checks#types-of-status-checks-on-github) along side the status check on GitHub. If you want to disable the commenting you can
set the environment variable `LINTR_COMMENT_BOT=false`.

If your project is in a subdirectory, and you would like to use GitHub Actions annotations, you can set `options(lintr.github_annotation_project_dir = "path/to/project")` which will add the path specified to the beginning of filenames in the annotations.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest wording it like "... which will make sure the annotations point to the correct files." or something like that, to describe the intention, not the technique. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yes that's a much better phrasing. Thanks

@jonkeane
Copy link
Contributor Author

Ok, I've pushed a few more updates. The path on windows is a bit off: it uses a mixture of \ and ////. I tried using normalizePath() to standardize these, but it ended up producing absolute paths which won't work on *nix (since the annotation paths AFAIK must be relative to the git checkout). I'm not sure if there is anyone using these annotations on GHA from a windows builder, so this might be ok. Thoughts?

@jonkeane
Copy link
Contributor Author

Is there anything else I can do to get this past the finish line?

Copy link
Collaborator

@AshesITR AshesITR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@AshesITR AshesITR merged commit 87078be into r-lib:master Oct 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants